Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

PVF: Document that preparation cannot lead to disputes #6873

Merged
merged 3 commits into from
Mar 15, 2023

Conversation

mrcnski
Copy link
Contributor

@mrcnski mrcnski commented Mar 13, 2023

PULL REQUEST

Overview

Since the changes in #6103, preparation should not be able to result in disputes.

  • Removed a comment referring to disputes in the context of deterministic vs. non-deterministic preparation errors.
  • Added a section to the implementer's guide presenting the rationale.
  • Added a warning for deterministic errors outside pre-checking, as they theoretically should not happen.

@mrcnski mrcnski added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. labels Mar 13, 2023
Copy link
Contributor

@s0me0ne-unkn0wn s0me0ne-unkn0wn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! And it's really good to have someone on the team who cares about documentation updates :)

Sidenote: While reading this, one more corner case came to mind. What if a PVF is pre-checked in one session and gets enacted in another session? Executor parameters may change in between. There is not much we can do about that, but it's worth considering.

@mrcnski
Copy link
Contributor Author

mrcnski commented Mar 14, 2023

@s0me0ne-unkn0wn I thought about that also. If executor params can affect preparation, then it would invalidate the assumptions documented here. Probably we would have to do pre-checking again when changing any parameter that can affect the runtime. Is there already an issue for this?

@burdges
Copy link
Contributor

burdges commented Mar 14, 2023

What executor params could be controlled?

@s0me0ne-unkn0wn
Copy link
Contributor

@burdges current list is like that:

  • Wasm executor max. memory size
  • Wasm stack limit
  • Native stack limit
  • PVF preparation timeouts: one for pre-check and another for execution
  • PVF execution timeouts: one for backing and another for approval voting/dispute participation etc.

Planned but not yet implemented:

@s0me0ne-unkn0wn
Copy link
Contributor

Probably we would have to do pre-checking again when changing any parameter that can affect the runtime

We can do that, but what's next? We cannon un-enact a PVF. Consider the situation when it's the first and only PVF for a parachain just onboarded.

Is there already an issue for this?

Just created paritytech/polkadot-sdk#694, let's continue there.

@burdges
Copy link
Contributor

burdges commented Mar 14, 2023

I suppose Wasm stack limit is a stack used by Wasm code, while native stack limit is used by the Wasm engine, our host calls, etc., yes? Aside form the two existing and planned preperation limits then these all sound more determined by the block than by the runtime, so not a new problem and not sure why they'd require rebuilding.

As for the two existing and planned preperation limits, are we instrumented to record the current max during preperation? I suppose no as code is rarely instrumented like this, but if so then we could just deactivate the PVF if the recorded limit gets invalidated on chain.

Anyways our problem is a preperation limit change could make preparing an already valid PVF impossible, which gives an adversary some control over who can check the PVF. Importantly, we should not enact a limit change immediately, just like we do not enact a PVF change immediately. Assuming all validators do rebuild, then yeah we'd prepare to unenact the PVF but governance should've time and ability to revert the limit change. Isn't this enough?

We could even have the vote block the limit change, so then governance should manually boot any fat parachains before reducing limits. Is that simpler? It's also kinda shitty that all validators must rebuild just to check a preperation limit violation.

@eskimor
Copy link
Member

eskimor commented Mar 15, 2023

We can do that, but what's next? We cannon un-enact a PVF. Consider the situation when it's the first and only PVF for a parachain just onboarded.

We could (not saying we should) don't accept execution environment parameter updates if pre-checking fails for a PVF that was valid before. So changing parameters would also be pre-checked, by re pre-checking all PVFs before enacting them. I actually think this is absolute overkill and does very likely more harm than good, but in that particular case we "could" do something. Realistically, I think we have to accept a small risk and live with it.

Copy link
Member

@eskimor eskimor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mrcnski !

@eskimor eskimor merged commit 5434628 into master Mar 15, 2023
@eskimor eskimor deleted the mrcnski/document-preparation-disputes branch March 15, 2023 00:35
@mrcnski
Copy link
Contributor Author

mrcnski commented Mar 15, 2023

As for the two existing and planned preperation limits, are we instrumented to record the current max during preperation? I suppose no as code is rarely instrumented like this, but if so then we could just deactivate the PVF if the recorded limit gets invalidated on chain.

@burdges Just to answer your question, we do have instrumentation for preparation memory usage and time taken. Agree with the rest of your post.

ordian added a commit that referenced this pull request Mar 16, 2023
* master: (50 commits)
  Issue 4393: Correcting Unnecessary Use of Arc (#6882)
  Companion for #13287  (#6655)
  timestamp ci job logs (#6890)
  Release parachain host API v4 (#6885)
  polkadot companion: #13128 (Pools commission) (#6264)
  companion for #13555 (#6842)
  Bump libgit2-sys from 0.14.1+1.5.0 to 0.14.2+1.5.1 (#6600)
  Bump bumpalo from 3.8.0 to 3.12.0 (#6599)
  Bump git2 from 0.16.0 to 0.16.1 (#6601)
  Council as SpendOrigin (#6877)
  PVF: Document that preparation cannot lead to disputes (#6873)
  Sync versions with current release (0.9.39) (#6875)
  Companion for paritytech/substrate#13592 (#6869)
  Update orchestra to the recent version (#6854)
  Delete unused Cargo.lock (#6870)
  Remove use of Store trait (#6835)
  Companion for Substrate #13564 (#6845)
  Adding Dispute Participation Metrics (#6838)
  Update `substrate` to 48e7cb1 (#6851)
  Move PVF timeouts to executor environment parameters (#6823)
  ...
ordian added a commit that referenced this pull request Mar 21, 2023
…slashing-client

* ao-past-session-slashing-runtime: (23 commits)
  Issue 4393: Correcting Unnecessary Use of Arc (#6882)
  Companion for #13287  (#6655)
  timestamp ci job logs (#6890)
  Release parachain host API v4 (#6885)
  polkadot companion: #13128 (Pools commission) (#6264)
  companion for #13555 (#6842)
  Bump libgit2-sys from 0.14.1+1.5.0 to 0.14.2+1.5.1 (#6600)
  Bump bumpalo from 3.8.0 to 3.12.0 (#6599)
  Bump git2 from 0.16.0 to 0.16.1 (#6601)
  Council as SpendOrigin (#6877)
  PVF: Document that preparation cannot lead to disputes (#6873)
  Sync versions with current release (0.9.39) (#6875)
  Companion for paritytech/substrate#13592 (#6869)
  Update orchestra to the recent version (#6854)
  Delete unused Cargo.lock (#6870)
  Remove use of Store trait (#6835)
  Companion for Substrate #13564 (#6845)
  Adding Dispute Participation Metrics (#6838)
  Update `substrate` to 48e7cb1 (#6851)
  Move PVF timeouts to executor environment parameters (#6823)
  ...
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants